chore(storage): Instrument existing metadata ops with storage trace package #11107
Conversation
Co-authored-by: Brenna N Epp <[email protected]>
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
product-auto-label
bot
added
the
api: storage
|
@BrennaEpp could you PTAL and help check the CLA, thanks! |
| @@ -91,8 +90,8 @@ func (a *ACLHandle) Delete(ctx context.Context, entity ACLEntity) (err error) { | |||
|
|
|||
| // Set sets the role for the given entity. | |||
| func (a *ACLHandle) Set(ctx context.Context, entity ACLEntity, role ACLRole) (err error) { | |||
| ctx = trace.StartSpan(ctx, "cloud.google.com/go/storage.ACL.Set") | |||
| defer func() { trace.EndSpan(ctx, err) }() | |||
| ctx, _ = startSpan(ctx, "storage.ACL.Set") | |||
There was a problem hiding this comment.
So it looks like we're dropping the actual returned Span. I assume we will use this later if we want to start adding more context pieces?
Also, does it make sense to have a separate endSpan method vs. calling Span.End directly on the span that was returned from startSpan? I recognize that you are basically just replicating the internal package pattern currently...
storage/trace_test.go
Outdated
| e.Reset() | ||
| } | ||
|
|
||
| func TestTraceSpansEmulated(t *testing.T) { |
There was a problem hiding this comment.
Can you move this to client_test.go (where we have other emulator tests currently)?
There was a problem hiding this comment.
If we are grouping by emulator tests I'd suggest renaming the file where we keep them
storage/trace_test.go
Outdated
| } | ||
|
|
||
| skippedTraceMethods := []string{"storage.Bucket.AddNotification", "storage.Bucket.Notifications", "storage.Bucket.DeleteNotification"} | ||
| for spanName, fn := range traceMethods { |
There was a problem hiding this comment.
I think this bespoke stuff shouldn't be necessary if you leverage the multi-transport harness that's already available in client_test?
storage/trace_test.go
Outdated
| // traceMethods are library methods that have trace instrumentation. This is a map whose keys are | ||
| // a string describing the spanName (e.g. storage.Bucket.Attrs) and values are functions which | ||
| // wrap library methods that implement the API calls. | ||
| var traceMethods = map[string]traceFunc{ |
There was a problem hiding this comment.
I think we could probably just move this in-line to the test (making it a table driven test)? It is a little awkward to read separately.
storage/trace.go
Outdated
| if !isOTelTracingDevEnabled() { | ||
| ctx = internalTrace.StartSpan(ctx, name) | ||
| return ctx, nil | ||
| } | ||
| opts = append(opts, getCommonTraceOptions()...) | ||
| ctx, span := tracer().Start(ctx, name, opts...) |
There was a problem hiding this comment.
Just to be clear, we are leaving out cloud.google.com/go/ from the trace name?
I'd suggest at least adding it back to the internalTrace implementation.
Update: supersedes #11051